-
Notifications
You must be signed in to change notification settings - Fork 514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Folia support #2701
base: master
Are you sure you want to change the base?
Folia support #2701
Conversation
Related issue: #2702 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, thank you. Adding support for Folia would be great and this is already a big step forward.
Here are some of my opinions for discussion.
Breaking changes
At this stage there are a lot of breaking changes in your code. Either we should decide to drop support for them or add a fallback. For example:
teleportAsync
is introduced by Paper. By adding the Folia API, it hides the unavailability of such methods from like Spigot. Maven multi modules could fix that, but could be overkill for this project. However, then requires such manual checks by the programmer and not be automatic by the compiler.
Thread-safety
Nevertheless, the biggest issue will be the topic around thread-safety. Before, we could expect that every statement scheduled from us or Bukkit to run on the main thread is not running concurrently with another. The introduction of multi region threading breaks this assumption.
Taking the examples for Folia project with some comments:
General rules of thumb:
Commands for entities/players are called on the region which owns the entity/player. Console commands are executed on the global region.
Implies: Commands invoked by players are now scattered across multi threads
Required action: Every statement accessing or changing AuthMe internal state have to introduce thread-safety mechanics.
Example: Verification command accessing shared HashSet
that is not thread-safe. It could be accessed by multiple players on different regions.
Events involving a single entity (i.e player breaks/places block) are called on the region owning entity. Events involving actions on an entity (such as entity damage) are invoked on the region owning the target entity.
Similar to commands
Existing thread-safety assumptions
Similar what about existing thread-safe methods. AFAIK Player.sendMessage
was declared as thread-safe. Do we now require that this method should accessed on the EntityScheduler
? Taking our MessageTask
should then no longer run asynchronous, but on this scheduler.
Proposal
All this likely requires huge refactoring, maybe separated into multiple PRs, before we could be safe to declare Folia support. The existing internal needs to be refactored to be thread-safe. For example locking/synchronizing the access on instance level, where possible.
Furthermore, it could be useful to add verification methods before we access anything player API (implemeneted as a Service?). So that we don't miss anything by accident. Something similar like Spigot already has for some API methods.
sendMessage(...) {
// thread-safe API access
if (currentThread == EntityScheduler || currentThread == player.getRegionThread()
}
It should be noted, that we still need to verify with the Spigot platform for potential deadlocks.
src/main/java/fr/xephi/authme/command/executable/authme/FirstSpawnCommand.java
Outdated
Show resolved
Hide resolved
The API is commented with "// Paper start", "// Paper end" around every new API method, and the Spigot API javadocs are easily navigable, probably it's easier to manually check than overengineering everything. |
Not everybody is looking at the source code. You are more likely look at the auto-complete suggestions of your favorite IDE. With the Maven modules, at least it could be verified by the compiler. |
Messages can be executed outside of any entity scheduler, since they do not interact with the world |
Folia has already implemented this checks for us, so we could simply add that methods to the BukkitService |
I made the verification methods available. By the way, keep in mind that they are not so useful, because Folia already throws exceptions if you call any method outside of its scheduler. |
Do you know if there is some kind of documentation about what is safe to be executed asynchronous? Spigot is such a huge project, it is hard to check if not a specific method accesses an internal component that isn't thread-safe. For example: The conversation and metadata (
So something like this already exists? That would be perfect, because such thing is better suited to be in Folia anyway. PlayerService {
public void setHealth(...) {
if (!currentThread == entityScheduler?/region thread?)
throw new IllegalStateException("Wrong thread");
}
} The checks you implemented could be unnecessary if Folia is aware of which thread is currently itself is running on. In other words your checks ( I'm sorry for all these questions, but I'm not that familiar with the project. Maybe it's also because Folia is still new and many things are planned. |
Folia doesn't allow to use the Bukkit scheduler, it has four different scheduler types, tasks must be scheduled on the correct ones.
I created two implementations of
BukkitService
, since Bukkit/Spigot/Paper API and Folia API are mutually incompatible.I marked the legacy scheduling methods as
@Deprecated
to make the tests compile, but they must be removed.I made fallback implementations for all the new Folia scheduling methods in the legacy Bukkit/Spigot/Paper
BukkitService
.I still haven't implemented the unit tests, I don't have time to re-implement most of them.